Repository navigation
fix(agc): mirror only unimported tails of registered heaps - #8380
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
AMD data point: RTX 3080 (NVIDIA 617.42) and AMD Radeon RDNA 2 iGPU ( AI-assisted: yes (Claude Code). |
|
@Zenodeon a merge-order note: #9514 changes the plain import in |
|
I am reviewing #9514, which changes |
adriadam10
left a comment
There was a problem hiding this comment.
#7292 is merged, so the first commit can be dropped; the branch needs a rebase onto current main, and it is not mechanical:
GuestBufferMemory.cppacquireMirrorconflicts in four places.mainnow accounts heap mirrors throughunlistMirror,reserveHeapBytes,ResidentBuffer::Createandmirror->accounted(#7647); this PR still usesheapMirrorsBeside(selected)andstate.heapBytes +=/-=. The partialselectedrange (address/bytes) has to go through the new reservation and error paths (heapMirrorError(range->address, range->bytes, held, ...),mirror->listed).- After rebasing, re-run the new
partialHeapImportBudgetTestswithANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1, validation layer, and check theMirrorCounters().heapBytesassertions still hold with resident (sparse-backed) mirrors. GuestBufferMemory.cpp:1694-1695: two blank lines afterimportPieces.- #9514 touches the same import site in
AcquireRegistered; @billythemenace2 confirmed it composes if #9514 lands first, so rebase over it if it merges before you push.
9a20ec2 to
5785179
Compare
adriadam10
left a comment
There was a problem hiding this comment.
The full Build on this head fails agc_driver_resident_heap_mirror on the Linux job (lavapipe, run 40011794592), in the new PartialImportTests:
ResidentHeapMirror.cpp:294:partial heap import: its mirror holds 131072 bytes, expected 65536. Log:host import budget: 0 MiB live of 0 MiB ... 1 imports (8 MiB) refused so far. The test is new in this PR, so main has no equivalent failure.- On lavapipe the sparse mirror of the tail accounts two blocks where the RTX 4070 run accounts one. Either the imported prefix or the sparse block granularity differs per driver, and the test pins
Blockfor the non-copied case.
Please check which it is: if the code selects a different prefix/tail on lavapipe, fix it there; if only the granularity differs, derive the expected bytes from the imported prefix and the device's sparse block size instead of the fixed Block. The copied case (expected = HeapBytes - 2 * Block) needs the same check.
|
Updated in 5785179: rebased onto main ea69535, dropped the merged #7292 prerequisite, routed selected mirror ranges through the current reservation/accounting and cleanup paths, and removed the extra blank lines. On Windows 11 / RTX 4070 (610.88), the same 16 focused tests passed on main and this head with required Vulkan, shader cache disabled and the validation layer confirmed loaded; neither run added VUID errors. Both new regressions fail on main as expected. A separate local combination with #9514 head f2e4290 passed 18/18 tests after adapting its added Thanks for catching the Linux CI failure in the new |
|
Fixed in 41d4aff. Reproduced the Linux failure: the lower-addressed write heap received the imported prefix, while the test incorrectly checked the read heap. It was allocation order, not sparse-block granularity. The regression now verifies the actual prefix and checks both allocation orders with sparse and copied mirrors. Updated head: 16/16 focused tests pass on Windows RTX 4070 and Linux lavapipe, with validation loaded and no VUID errors. Linux main also passes 16/16; the updated regression fails on main as expected. Full upstream CI still needs a rerun. |
|
Re-checked against current main: the new changes to |
What
When a registered heap exceeds the host-import budget, import the fitting prefix and mirror only its unimported tail. Previously a budget refusal could mirror the whole registration, including bytes already represented by an import. Keep the original registration identity for lifetime tracking while keying and allocating each mirror by its selected physical range.
Rebased onto main
ea695358ee1ccaf3d108b720d82c5d88413d43dc, dropping the merged #7292 prerequisite. This PR contains the standalone resource fix and a focused test follow-up. The partial mirror uses main's #7647 reservation and sparse-resident accounting:reserveHeapBytes,ResidentBuffer::Create,accounted,listedandunlistMirror, including reservation rollback and retirement. Removed the extra blank lines afterimportPieces.Related work: #9514 changes budget-refusal eviction before the address-copy cap. Agreed merge order is #9514 first, then rebase this PR onto the resulting main. Its implementation is excluded from this branch. The local combined build needs one bookkeeping adaptation in #9514's added retry path: replace
imported.push_back(range->address)withimported.emplace_back(range->address, range->address + range->bytes)to match this PR's imported-range tracking.Tested
libsand recorded local library hashes. No game or shader patches included.ea695358ee1ccaf3d108b720d82c5d88413d43dcand candidate41d4affd6191d6b799f838a8e56b216335b563a2: 16/16 passed on each platform (Windows RTX 4070 and Ubuntu 26.04.1 under WSL2, lavapipe / llvmpipe LLVM 21.1.8, Mesa 26.0.8). Linux main was rerun during this follow-up; Windows main results are from the unchanged pinned base's prior matched run. UsedANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationand verbose CTest with a 30-second individual timeout. Verified the layer actually loaded; all four logs have no missing-layer warnings, skipped tests orVUID-errors.57851797857808c16b0380ba08f8df118e02dc36, composed locally with fix(agc): an address-based build evicts idle imports instead of copying past the cap #9514 headf2e4290e9c75880fc964f56528414713d2e21e96and the imported-range bookkeeping adaptation above: 18/18 passed, addingagc_address_copy_capandagc_address_copy_cap_remembered. Actual validation layer loaded, no skipped tests, missing-layer warnings orVUID-errors. This composition predates the test-only follow-up and remains local.0x769c65ff0000had no imported prefix; the lower-addressed write heap0x769c657e0000had the 131072-byte prefix. The test incorrectly assumed allocation order equaled registry address order. This was not a sparse-block granularity difference. The follow-up verifies the exact prefix on the selected heap and tests both allocation orders with resident and copied fallback mirrors.the first registered heap has no exact two-block imported prefix. The earlier Windows-only mocked cases also fail on main (a heap exceeding the import budget did not import its fitting prefix). No Vulkan validation errors in the negative controls.partialHeapImportBudgetTestscovers a 384 KiB registration with a 256 KiB imported prefix and exact 128 KiB tail, mirror replacement, first/last bytes, CPU writes, GPU write-back, short final windows, neighbors and retirement. Added the simultaneous import-window and budget-limit case.git diff --checkandtools/check_conventions.py --base upstream/mainpass. The required--base origin/maininvocation reports inherited findings against the older fork main; those findings are outside this topic diff.Checklist
main; no other open PR implements the same functions — related fix(agc): an address-based build evicts idle imports instead of copying past the cap #9514 composition and merge order described aboveNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)